-
Notifications
You must be signed in to change notification settings - Fork 7.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ZOOKEEPER-3832 ZKHostnameVerifier rejects valid certificates with subjectAltNames #1353
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, +1
I had one question about ASN.1 DER, please take a look
if (o instanceof String) { | ||
result.add(new SubjectName((String) o, type)); | ||
} else if (o instanceof byte[]) { | ||
// TODO ASN.1 DER encoded form |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what ASN.1 DER is or how commonly it is used, but I think at least printing out a warning here would make sense (informing the user that ASN.1 DER is not supported). (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it can be done with BouncyCastle ASN1 libraries, but this part was missing in the original patch too. I'd be happy to add it as a separate ticket, but first I need an example certificate with ASN1 encoded data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Mate here, probably adding a warning until this TODO is not implemented would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a static method. How can I log here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't see it's a static method. But I believe you can log by making the logger also static. Not sure it is worth it though, it's not a stopper from my side if we leave the TODO, just a nice-to-have.
zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKHostnameVerifierTest.java
Show resolved
Hide resolved
zookeeper-server/src/test/java/org/apache/zookeeper/common/CertificatesToPlayWith.java
Show resolved
Hide resolved
retest maven build |
2 similar comments
retest maven build |
retest maven build |
@eolivelli @nkalmar @symat Maven build is green now. Would you like me to add some logging before submitting? |
I think you can push it as it is. Nice change! |
Merging it now |
…bjectAltNames This issue has been reported by a user who wanted to use a cert that contains SAN entries that are not of type DNS or IP. I've come across the following ticket in http client project which seems to be related: https://issues.apache.org/jira/browse/HTTPCLIENT-1906 This is the backport of the fix. Original patch: apache/httpcomponents-client@56cc245 Target versions: 3.5, 3.6, 3.7 Author: Andor Molnar <andor@apache.org> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org> Closes #1353 from anmolnar/ZOOKEEPER-3832 (cherry picked from commit 5820d10) Signed-off-by: Enrico Olivelli <eolivelli@apache.org>
Thanks @eolivelli |
I have written in JIRA. |
I'll create separate PR. |
…bjectAltNames This issue has been reported by a user who wanted to use a cert that contains SAN entries that are not of type DNS or IP. I've come across the following ticket in http client project which seems to be related: https://issues.apache.org/jira/browse/HTTPCLIENT-1906 This is the backport of the fix. Original patch: apache/httpcomponents-client@56cc245 Target versions: 3.5, 3.6, 3.7 Author: Andor Molnar <andor@apache.org> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org> Closes apache#1353 from anmolnar/ZOOKEEPER-3832
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
NVM, it's already committed :) |
…bjectAltNames This issue has been reported by a user who wanted to use a cert that contains SAN entries that are not of type DNS or IP. I've come across the following ticket in http client project which seems to be related: https://issues.apache.org/jira/browse/HTTPCLIENT-1906 This is the backport of the fix. Original patch: apache/httpcomponents-client@56cc245 Target versions: 3.5, 3.6, 3.7 Author: Andor Molnar <andor@apache.org> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org> Closes apache#1353 from anmolnar/ZOOKEEPER-3832
…bjectAltNames This issue has been reported by a user who wanted to use a cert that contains SAN entries that are not of type DNS or IP. I've come across the following ticket in http client project which seems to be related: https://issues.apache.org/jira/browse/HTTPCLIENT-1906 This is the backport of the fix. Original patch: apache/httpcomponents-client@56cc245 Target versions: 3.5, 3.6, 3.7 Author: Andor Molnar <andor@apache.org> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org> Closes apache#1353 from anmolnar/ZOOKEEPER-3832
…bjectAltNames This issue has been reported by a user who wanted to use a cert that contains SAN entries that are not of type DNS or IP. I've come across the following ticket in http client project which seems to be related: https://issues.apache.org/jira/browse/HTTPCLIENT-1906 This is the backport of the fix. Original patch: apache/httpcomponents-client@56cc245 Target versions: 3.5, 3.6, 3.7 Author: Andor Molnar <andor@apache.org> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org> Closes apache#1353 from anmolnar/ZOOKEEPER-3832
…bjectAltNames This issue has been reported by a user who wanted to use a cert that contains SAN entries that are not of type DNS or IP. I've come across the following ticket in http client project which seems to be related: https://issues.apache.org/jira/browse/HTTPCLIENT-1906 This is the backport of the fix. Original patch: apache/httpcomponents-client@56cc245 Target versions: 3.5, 3.6, 3.7 Author: Andor Molnar <andor@apache.org> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org> Closes apache#1353 from anmolnar/ZOOKEEPER-3832
…bjectAltNames This issue has been reported by a user who wanted to use a cert that contains SAN entries that are not of type DNS or IP. I've come across the following ticket in http client project which seems to be related: https://issues.apache.org/jira/browse/HTTPCLIENT-1906 This is the backport of the fix. Original patch: apache/httpcomponents-client@56cc245 Target versions: 3.5, 3.6, 3.7 Author: Andor Molnar <andor@apache.org> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org> Closes apache#1353 from anmolnar/ZOOKEEPER-3832
…bjectAltNames This issue has been reported by a user who wanted to use a cert that contains SAN entries that are not of type DNS or IP. I've come across the following ticket in http client project which seems to be related: https://issues.apache.org/jira/browse/HTTPCLIENT-1906 This is the backport of the fix. Original patch: apache/httpcomponents-client@56cc245 Target versions: 3.5, 3.6, 3.7 Author: Andor Molnar <andor@apache.org> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org> Closes apache#1353 from anmolnar/ZOOKEEPER-3832 Change-Id: I5d3c0d66010942a252cb9f5cd08fa50eadd5925f
This issue has been reported by a user who wanted to use a cert that contains SAN entries that are not of type DNS or IP.
I've come across the following ticket in http client project which seems to be related:
https://issues.apache.org/jira/browse/HTTPCLIENT-1906
This is the backport of the fix.
Original patch: apache/httpcomponents-client@56cc245
Target versions: 3.5, 3.6, 3.7